I had a weird bug that I was dealing with where I had added a new GeckoProgram target in a directory where Makefile.in didn't state NSDISTMODE=copy. As a result, I got a bunch of really weird XPCOM initialisation failures when trying to launch the resulting binary. Apparently this is a known issue when an executable is symlinked rather than copied? I think that GeckoProgram should imply copying rather than symlinking.

This would let us remove all the hardcoded `NSDISTMODE = copy` bits in Makefile.in.

I think the right thing to do is to link the programs in their final destination directly instead of linking them in the objdir and then copying/symlinking them. The only thing of note is that on windows, that means the pdb would go alongside, but there's a flag to make the linker put the pdb elsewhere.

(In reply to Mike Hommey [:glandium] from comment #2) > I think the right thing to do is to link the programs in their final > destination directly instead of linking them in the objdir and then > copying/symlinking them. The only thing of note is that on windows, that > means the pdb would go alongside, but there's a flag to make the linker put > the pdb elsewhere. I agree. I actually thought about this as I was working on bug 1253436. The fact we can produce JSON describing binaries means the backend has enough knowledge to do this today.

I was worried that this would be hard because we used to clobber dist/bin at the beginning of the build, but I guess we don't do that anymore, so this should be very doable.

Comment on attachment 8931097 [details] bug 1255485 - build PROGRAMs directly in dist/bin instead of copying them. https://reviewboard.mozilla.org/r/202178/#review207560 You're right, that was surprisingly pleasant. ::: config/rules.mk:815 (Diff revision 1) > ifeq (,$(filter 1,$(MOZ_AUTOMATION_BUILD_SYMBOLS))) > DUMP_SYMS_TARGETS := > endif > endif > > -$(foreach file,$(DUMP_SYMS_TARGETS),$(eval $(call syms_template,$(file),$(file)_syms.track))) > +$(foreach file,$(DUMP_SYMS_TARGETS),$(eval $(call syms_template,$(file),$(notdir $(file))_syms.track))) Consider pushing this stripping up to the definition of DUMP_SYMS_TARGETS, since it's not 100% clear that it'll do the right thing for the other components.

Comment on attachment 8931098 [details] bug 1255485 - Remove NSDISTMODE=copy from Makefiles. https://reviewboard.mozilla.org/r/202180/#review207564 Stamp!

Comment on attachment 8931099 [details] bug 1255485 - Remove NSDISTMODE=copy support from config.mk. https://reviewboard.mozilla.org/r/202182/#review207566 Stamp!

Doesn't this result in the pdbs being created in dist/bin too? I remember that was one of the problems I had when I tried this a while ago.

(In reply to Mike Hommey [:glandium] from comment #12) > Doesn't this result in the pdbs being created in dist/bin too? I remember > that was one of the problems I had when I tried this a while ago. It might, I'll have to check. That's fixable.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=9d2bb9f378757eec2682cdc9e9f7f449d3585462 I apparently chose a bad inbound changeset for this, because all my Windows builds are busted. I rebased it and it builds fine on Windows locally. (In reply to Mike Hommey [:glandium] from comment #12) > Doesn't this result in the pdbs being created in dist/bin too? I remember > that was one of the problems I had when I tried this a while ago. No, it turns out we already set LINK_PDBFILE to be just the basename of the file, so that works OK: https://dxr.mozilla.org/mozilla-central/rev/da90245d47b17c750560dedb5cbe1973181166e3/config/rules.mk#166 https://dxr.mozilla.org/mozilla-central/rev/da90245d47b17c750560dedb5cbe1973181166e3/config/rules.mk#557 However, after doing a local Windows build I notice that I do wind up with .ilk and .lib files in dist/bin, so I'm going to see if it's possible to make those go elsewhere. (I'm actually sorta confused as to why we wind up with .lib files for binaries, TBH!)

We apparently get .lib and .exp files generated because we must have functions that are __declspec(dllexport) that wind up in firefox.exe. (I don't know if that's intentional or not.) I should be able to make those not wind up in dist/bin. Unfortunately there's no way to control the location of the .ilk files, which are used for incremental linking, so we're going to be stuck with those. They won't be produced in most automation builds, since we use compiler options that disable incremental linking, so it hopefully shouldn't cause any issues even if we want to make packaging better in the future.

Comment on attachment 8931097 [details] bug 1255485 - build PROGRAMs directly in dist/bin instead of copying them. https://reviewboard.mozilla.org/r/202178/#review207560 > Consider pushing this stripping up to the definition of DUMP_SYMS_TARGETS, since it's not 100% clear that it'll do the right thing for the other components. I looked into doing this but I can't because `syms_template` does need the full path to the .exe file so it can pass that to dump_syms.

One thing I still need to fix--on Windows this breaks symbol dumping for programs because the exe and pdb files are no longer in the same directory. This should be fixable, the full path to the pdb file is in fact in the exe headers.

Comment on attachment 8932082 [details] bug 1255485 - force import libraries to be generated in objdir, not dist/bin. https://reviewboard.mozilla.org/r/203138/#review211156

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #21) > One thing I still need to fix--on Windows this breaks symbol dumping for > programs because the exe and pdb files are no longer in the same directory. > This should be fixable, the full path to the pdb file is in fact in the exe > headers. The latest try push contains a simple patch atop the existing patches here that should fix this issue. If that works then it should be easy to get that reviewed and land all of these patches!

OK that try push exposed two more issues: 1) I failed to fix the integration test in unit-symbolstore.py to match the change to locate pdb files, so that test failed. 2) On Mac builds, some programs wind up using DIST_SUBDIR to get installed inside an app bundle, but nothing was ensuring that that subdirectory was created before linking happened (I added it as a prerequisite to the PROGRAM rule in rules.mk): https://dxr.mozilla.org/mozilla-central/rev/e4107773cffb1baefd5446666fce22c4d6eb0517/toolkit/crashreporter/minidump-analyzer/moz.build#28

At least for building programs into dist/bin, it would be splendid if there were some tests for this new behavior.

(In reply to Nathan Froyd [:froydnj] from comment #31) > At least for building programs into dist/bin, it would be splendid if there > were some tests for this new behavior. Good call. I wrote a frontend test and a recursive make backend test to check the various things that can impact the program destination. I pushed to try again to make sure the tests pass on all platforms.

Comment on attachment 8931097 [details] bug 1255485 - build PROGRAMs directly in dist/bin instead of copying them. https://reviewboard.mozilla.org/r/202178/#review220286 OK, I re-reviewed this. It looks fine to me, and my questions are truly questions and not blockers. Green try is good to go! ::: browser/app/Makefile.in:49 (Diff revision 3) > > ifneq (,$(filter-out WINNT,$(OS_ARCH))) > > ifdef COMPILE_ENVIRONMENT > libs:: > - cp -p $(MOZ_APP_NAME)$(BIN_SUFFIX) $(DIST)/bin/$(MOZ_APP_NAME)-bin$(BIN_SUFFIX) > + cp -p $(DIST)/bin/$(MOZ_APP_NAME)$(BIN_SUFFIX) $(DIST)/bin/$(MOZ_APP_NAME)-bin$(BIN_SUFFIX) Huh. Isn't this saying that we're _not_ actually installing into the correct place? Can we not fold this into the program declaration and avoid this copy? (This could be follow-up.) ::: config/rules.mk:555 (Diff revision 3) > > # > # PROGRAM = Foo > # creates OBJS, links with LIBS to create Foo > # > -$(PROGRAM): $(PROGOBJS) $(STATIC_LIBS_DEPS) $(EXTRA_DEPS) $(RESFILE) $(GLOBAL_DEPS) > +$(PROGRAM): $(PROGOBJS) $(STATIC_LIBS_DEPS) $(EXTRA_DEPS) $(RESFILE) $(GLOBAL_DEPS) $(call mkdir_deps,$(FINAL_TARGET)) I know you noted why you're doing this, but it's special and opaque. Can we not determine the directory from `$(PROGRAM)`? We definitely produce some executables that aren't in `dist/bin`, at least on macOS (crashreporter?)

Comment on attachment 8943303 [details] bug 1255485 - add some tests for building programs in dist/bin. https://reviewboard.mozilla.org/r/213608/#review220292 Looks great.

Comment on attachment 8931097 [details] bug 1255485 - build PROGRAMs directly in dist/bin instead of copying them. https://reviewboard.mozilla.org/r/202178/#review220286 > Huh. Isn't this saying that we're _not_ actually installing into the correct place? Can we not fold this into the program declaration and avoid this copy? > > (This could be follow-up.) This is the horrible "we used to ship `firefox-bin` as the binary, and `firefox` as a shell script, then we made `firefox` the binary, but lots of people still expect to invoke `firefox-bin` so we just ship a copy named that" thing. It would be nice to get rid of this but it doesn't feel very high priority to me. > I know you noted why you're doing this, but it's special and opaque. Can we not determine the directory from `$(PROGRAM)`? We definitely produce some executables that aren't in `dist/bin`, at least on macOS (crashreporter?) We could, sure, but for programs that don't get dist installed at all it'd just be `.`. For things like that, the `FINAL_TARGET` is correctly set to that path: https://dxr.mozilla.org/mozilla-central/rev/9fe69ff0762da191fbd2b9fc0718e96f1ab4137e/toolkit/crashreporter/minidump-analyzer/moz.build#28 (this was what broke and the reason I need this `mkdir_deps`). Surprisingly the crashreporter client doesn't set `FINAL_TARGET`, it just uses the same ugly Makefile rules that browser/app does to build its app bundle (bummer).

Pushed by tmielczarek@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/51af06b6123c build PROGRAMs directly in dist/bin instead of copying them. r=nalexander https://hg.mozilla.org/integration/autoland/rev/0ba36c0feddb Remove NSDISTMODE=copy from Makefiles. r=nalexander https://hg.mozilla.org/integration/autoland/rev/ed6dd4aefadb Remove NSDISTMODE=copy support from config.mk. r=nalexander https://hg.mozilla.org/integration/autoland/rev/5672cf8d324b force import libraries to be generated in objdir, not dist/bin. r=chmanchester https://hg.mozilla.org/integration/autoland/rev/90b7449882b6 add some tests for building programs in dist/bin. r=nalexander

Backed out 5 changesets (bug 1255485) for mochitest failures on test/mochitest/test_hangui.xul Backout: https://hg.mozilla.org/integration/autoland/rev/336ec3ad9df4be2aab93580018a50270173439dc Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=90b7449882b65a5f16e75c58106f6cf153785f2f Log: https://treeherder.mozilla.org/logviewer.html#?job_id=157844683&repo=autoland&lineNumber=3566 23:42:56 INFO - Buffered messages finished 23:42:56 ERROR - 297 INFO TEST-UNEXPECTED-FAIL | dom/plugins/test/mochitest/test_hangui.xul | Test timed out. 23:42:56 INFO - reportError@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:121:7

I stumbled on a fix for this that looks good on try, will upload shortly.

Comment on attachment 8959720 [details] Bug 1255485 - Don't assume target path is srcdir relative when locating a program's manifest on windows. https://reviewboard.mozilla.org/r/228476/#review234380

Comment on attachment 8959720 [details] Bug 1255485 - Don't assume target path is srcdir relative when locating a program's manifest on windows. Thanks for fixing this! I keep meaning to file a bug, we should really rip out most of that manifest-embedding code and simplify it.

Pushed by cmanchester@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad09b92fb875 build PROGRAMs directly in dist/bin instead of copying them. r=nalexander https://hg.mozilla.org/integration/mozilla-inbound/rev/6fffc6fbb8c9 Remove NSDISTMODE=copy from Makefiles. r=nalexander https://hg.mozilla.org/integration/mozilla-inbound/rev/5888578c22f8 Remove NSDISTMODE=copy support from config.mk. r=nalexander https://hg.mozilla.org/integration/mozilla-inbound/rev/465358fef0d2 force import libraries to be generated in objdir, not dist/bin. r=chmanchester https://hg.mozilla.org/integration/mozilla-inbound/rev/02e71426ed3e add some tests for building programs in dist/bin. r=nalexander https://hg.mozilla.org/integration/mozilla-inbound/rev/7862033a42ab Don't assume target path is srcdir relative when locating a program's manifest on windows. r=ted

https://hg.mozilla.org/mozilla-central/rev/ad09b92fb875 https://hg.mozilla.org/mozilla-central/rev/6fffc6fbb8c9 https://hg.mozilla.org/mozilla-central/rev/5888578c22f8 https://hg.mozilla.org/mozilla-central/rev/465358fef0d2 https://hg.mozilla.org/mozilla-central/rev/02e71426ed3e https://hg.mozilla.org/mozilla-central/rev/7862033a42ab